Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix MakeCredential return value error #17

Closed
wants to merge 1 commit into from

Conversation

Zha0Chan
Copy link

Return value in
func MakeCredential(rand io.Reader, key *tpm2.Public, credential tpm2.Digest, objectName tpm2.Name) (credentialBlob tpm2.IDObject, secret tpm2.EncryptedSecret, err error)

The prototypes of credentialBlob tpm2.IDObject and secret tpm2.EncryptedSecret are respectively IDObject corresponding to the TPM2B_ID_OBJECT type. EncryptedSecret corresponds to the TPM2B_ENCRYPTED_SECRET type.
The prototypes of these two types are as follows

/* Definition of TPM2B_ID_OBJECT Structure <INOUT> */
typedef struct {
    UINT16 size;
    BYTE credential[sizeof(TPMS_ID_OBJECT)];
} TPM2B_ID_OBJECT;
/* Definition of TPM2B_ENCRYPTED_SECRET Structure */
typedef struct {
    UINT16 size;
    BYTE secret[sizeof(TPMU_ENCRYPTED_SECRET)];
} TPM2B_ENCRYPTED_SECRET;

The first two bytes in each structure identify the length of the content, which is not included in the original code, so the fix is as follows:

func MakeCredential(rand io.Reader, key *tpm2.Public, credential tpm2.Digest, objectName tpm2.Name) (credentialBlob tpm2.IDObject, secret tpm2.EncryptedSecret, err error) {
...
credentialBlob, err = mu.MarshalToBytes(credentialBlob)
if err != nil {
return nil, nil, fmt.Errorf("cannot marshal credential bytes: %w", err)
}
	
secret, err = mu.MarshalToBytes(secret)
if err != nil {
return nil, nil, fmt.Errorf("cannot marshal secret bytes: %w", err)
}
return credentialBlob, secret, nil
}

@chrisccoulson chrisccoulson self-requested a review February 12, 2025 09:41
Copy link
Collaborator

@chrisccoulson chrisccoulson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, sorry for taking a long time to get to this - I hadn't noticed it because I don't regularly check the PRs for this repo and I wasn't directly assigned as a reviewer.

The current code is correct - I think you might be a bit confused about how we handle TPM2B byte buffer types. First of all, we have a set of tests of TPMContext.ActivateCredential which make use of objectutil.MakeCredential in order to provide a credential to test. If you run one of these with this PR applied, these tests will fail, eg:

$ go test . -v -check.v -use-mssim -test.run "^Test$" -check.f "objectSuite\.TestActivateCredential"
=== RUN   Test

----------------------------------------------------------------------
FAIL: cmds_object_test.go:537: objectSuite.TestActivateCredential

cmds_object_test.go:538:
    s.testActivateCredential(c, &testActivateCredentialData{})
cmds_object_test.go:528:
    c.Check(err, IsNil)
... value *tpm2.TPMParameterError = &tpm2.TPMParameterError{TPMError:(*tpm2.TPMError)(0xc00024cfa0), Index:2} ("TPM returned an error for parameter 2 whilst executing command TPM_CC_ActivateCredential: TPM_RC_SIZE (structure is the wrong size)")

cmds_object_test.go:538:
    s.testActivateCredential(c, &testActivateCredentialData{})
cmds_object_test.go:529:
    c.Check(certInfo, DeepEquals, Digest(credential))
... obtained tpm2.Digest = tpm2.Digest(nil)
... expected tpm2.Digest = tpm2.Digest{0x73, 0x65, 0x63, 0x72, 0x65, 0x74, 0x20, 0x63, 0x72, 0x65, 0x64, 0x65, 0x6e, 0x74, 0x69, 0x61, 0x6c}
... Difference:
...     tpm2.Digest[0] != tpm2.Digest[17]


OOPS: 0 passed, 1 FAILED
--- FAIL: Test (0.65s)
FAIL
FAIL    github.com/canonical/go-tpm2    1.670s
FAIL

TPM2B types are implemented as plain go slices (see the documentation for how we map go types to TPM types when serializing to / deserializing from the TPM wire format), although TPM2B types that encapsulate a TPMS or TPMT type are handled a bit differently. These types are not carried around in go with an explicit size field (well, technically they are by the go runtime because every slice has a header describing its length and capacity, but those are implementation details of the go runtime).

When you serialize a byte slice ([]byte or any other type based on this) to the TPM wire format using one of the MarshalTo* functions from the mu sub-package, it automatically prepends the 2 byte size field in the resulting data.

So, whilst you're correct that the TPM2B_ID_OBJECT and TPM2B_ENCRYPTED_SECRET are implemented with a 2 byte size field, the corresponding go type for each is just a plain byte slice. They get a size field when serialized to the TPM wire format using mu.MarshalToBytes or mu.MarshalToWriter (and inversely, when deserializing back to the go types with mu.UnmarshalFromBytes or mu.UnmarshalFromReader, what you get back is a plain go byte slice (or some type that's derived from []byte) with the size field removed.

You'll notice that earlier in the objectutil.MakeCredential function, we do pass the supplied credential through a call to mu.MarshalToBytes (the result of which is referred to as credentialBlob). This is because the outer wrapper that's applied has to be applied to the entire TPM2B structure - including its size field, so we have to serialize it to the TPM wire format first.

All this PR does is result in the function returning credentialBlob and secret slices each with a 2 byte size field prepended to the slice payload, which isn't necessary. When passing these results through mu.MarshalToBytes again (which happens at some point when calling TPMContext.ActivateCredential), all that happens is you get a new TPM2B wrapper with an extra uint16 size field around a slice that already indicates a size in its first 2 bytes. On the TPM size, the deserialization code uses the outer 2 bytes to deserialize the supplied buffers, resulting in buffers that contain 2 extra bytes being passed to the action code. And in the above case for TPM2_ActivateCredential, because it is using RSA-OAEP, it results in it failing and returning a TPM_RC_SIZE error for the secret parameter because the size of it is no longer consistent with the size of the RSA key.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants